fix perf regression from abby canonical form - #162531
Conversation
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fix perf regression from abby canonical form
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (6045edf): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 480.775s -> 481.555s (0.16%) |
|
looks like when I said
when under PGO and whatnot instead of my local machine, seeeems the first point is more like 15% rather than 70%. (this PR is fixing just the first point, and seems to come back as recovering 15% of the original regression) I don't know how to read the online web report, but running these locally: cargo build --release -p collector && ./target/release/collector profile_local cachegrind +4fcf39725a9c99bd495d8c73af83628a256ff9a9 --rustc2 +d8df82673d5911b6112a85bf91d9adefb2c66a1a --exact-match libc-0.2.172 --profiles Check --scenarios Full # original regression
cargo build --release -p collector && ./target/release/collector profile_local cachegrind +55c4dfed758e741620e3320ed472ff5c4140856a --rustc2 +6045edfc980d86d93057e102a0447621562f9c8b --exact-match libc-0.2.172 --profiles Check --scenarios Full # this PRshows the first one as + still, this PR came back fully green, so I'm down to merge this as-is, I'll undraft this. I'll try to poke around and investigate further to remove the alloc on the |
|
|
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
pushed option B in the PR description, will do a perf run on that too. since yeah, basically what panstromek said in zulip:
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fix perf regression from abby canonical form
This comment has been minimized.
This comment has been minimized.
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
@bors p=10 |
This comment has been minimized.
This comment has been minimized.
fix perf regression from abby canonical form #161306 regressed performance on stable I believe all the perf impact is due to this https://github.com/rust-lang/rust/blob/d8df82673d5911b6112a85bf91d9adefb2c66a1a/compiler/rustc_infer/src/infer/mod.rs#L186 before, `SolverRegionConstraintStorage` was a dummy simple thing, no allocations. now, it's a `RegionConstraint { and: Box([]), or: Box([Box([])]) }` the majority of the perf impact (70%ish I think) is because the compiler is not sufficiently smart to optimize `And::new([])` into `And(Box::new([]))` (the former does a bunch of `IndexSet` allocations and stuff, the latter is a no-op, just a nullptr plus zero length metadata) the the rest of the perf impact (30%ish) is due to the `or` case allocating the `Box([Box([])])`, it's not just a nullptr+zero options to fix: - option A: just fix the `And::new([])` being terrible - option B: option A, *and also*, `SolverRegionConstraintStorage` stores an `Option` that is lazily init on first access, to prevent the perf hit from the `or` case - option C: option A, *and also*, use some kind of `SmallVec` something or other to make the `or` case be zero-alloc. I have not profiled this due to it being an invasive change and effort, this might not actually fix the perf. This PR is out option A to see if it actually works with the full perf machinery with PGO and whatnot instead of just on my machine (I am very inexperienced with perf testing!). It might be the case that full PGO blah blah *is* actually sufficiently smart to optimize `And::new([])`, and the actual perf diff is due to the `or` case (which I'm calling the 30%ish impact, might be actually 100%), in which case this PR should produce a no-op perf diff. r? @BoxyUwU
|
💔 Test for 5261ebd failed: CI. Failed job:
|
|
@bors treeclosed=10 Runner problem git not found on EC2 runners |
|
Tree closed for PRs with priority less than 10. |
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
This comment has been minimized.
This comment has been minimized.
|
@bors treeopen |
|
Tree is now open for merging. |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 018018e (parent) -> 67eda61 (this PR) Test differencesShow 10 test diffs10 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 67eda617e6a8f8ecec01e1ba7fafe2072a64adcc --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (67eda61): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.5%, secondary -2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 479.823s -> 476.479s (-0.70%) |
View all comments
#161306 regressed performance on stable
I believe all the perf impact is due to this
rust/compiler/rustc_infer/src/infer/mod.rs
Line 186 in d8df826
before,
SolverRegionConstraintStoragewas a dummy simple thing, no allocations.now, it's a
RegionConstraint { and: Box([]), or: Box([Box([])]) }the majority of the perf impact (70%ish I think) is because the compiler is not sufficiently smart to optimize
And::new([])intoAnd(Box::new([]))(the former does a bunch ofIndexSetallocations and stuff, the latter is a no-op, just a nullptr plus zero length metadata)the the rest of the perf impact (30%ish) is due to the
orcase allocating theBox([Box([])]), it's not just a nullptr+zerooptions to fix:
And::new([])being terribleSolverRegionConstraintStoragestores anOptionthat is lazily init on first access, to prevent the perf hit from theorcaseSmallVecsomething or other to make theorcase be zero-alloc. I have not profiled this due to it being an invasive change and effort, this might not actually fix the perf.This PR is out option A to see if it actually works with the full perf machinery with PGO and whatnot instead of just on my machine (I am very inexperienced with perf testing!). It might be the case that full PGO blah blah is actually sufficiently smart to optimize
And::new([]), and the actual perf diff is due to theorcase (which I'm calling the 30%ish impact, might be actually 100%), in which case this PR should produce a no-op perf diff.r? @BoxyUwU